Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add:framework-zap #42

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

add:framework-zap #42

wants to merge 13 commits into from

Conversation

Sacramento-20
Copy link
Contributor

Pull request created, all basic information about the program's flow can be found in the README.

@Sacramento-20 Sacramento-20 requested a review from cunha September 5, 2024 17:42

# Verifica se o diretório de origem existe
if [ ! -d "$ORIGEM" ]; then
exit 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please log an error message so the user knows what has gone wrong

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4ae37b6
logs were included in possible errors in copy.sh

Suggested change
exit 1
# Check if the source directory exists
if [ ! -d "$SOURCE" ]; then
echo "Error: Source directory '$SOURCE' does not exist."

@@ -0,0 +1,35 @@
#!/bin/bash

# Verifica se foi passado somente 1 argumento
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please translate all variable names, documentation, function names into English.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all scripts and documentation have been translated into English

@@ -0,0 +1,35 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#!/bin/bash
#!/bin/bash
set -eu

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested change was implemented.

fi

# Verifica se o diretório de destino existe se não existir, criar
docker compose exec framework bash -c "mkdir -p $DESTINO && rm -rf $DESTINO/*"
Copy link
Member

@cunha cunha Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating the mount point is better done inside the Dockerfile, as it's something the container needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't implement the functionality satisfactorily.

# comando que apaga todos os arquivos de configuração se houver antes de copiar os novos.

# Copia apenas o conteúdo .json do diretório de origem para o volume
for file in "$ORIGEM"/*.json; do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this step. The docker-compose.yml file is currently defining a named Docker volume (shared_data) and mounting it inside the container on /shared_data. We should instead mount a local directory inside the container (possibly dropping the underscore for conciseness), e.g.:

framework:
  volumes:
    - ./framework:/app
    - ./shared:/shared:rw

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing it the way you suggested, as we discussed, but it didn't work as well as the current approach, where the user provides the folder with the information and the script copies it to the volume.

- zaproxy
volumes:
- ./framework:/app
- shared_data:/shared_data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Share local directory for input/output (replace copy.sh script, see comments in there).

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to change it, but the new approach didn't work correctly, so I kept the original one.

@@ -0,0 +1,20 @@
generate_key:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like file.env is never used. Where is this used?

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .env file is created to temporarily store the API key for Zap. After the configuration, a command is included in the .sh script to delete the file.

Suggested change
generate_key:
rm file.env

password: str
```

Apenas dois atributos são opcionais: a URL da página de login e o título do relatório. Se a URL de login não for fornecida, o framework utiliza o spider do ZAP para tentar identificar as URLs da aplicação a partir da URL base. No entanto, nem sempre é possível encontrar a página de login automaticamente, por isso é recomendável fornecer essa URL sempre que possível. Se o spider falhar em encontrar a URL de login, um erro será lançado.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O que é o "título do relatório"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "context"?

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation was revised, and all the mentioned concepts, as well as those that previously lacked explanations, were properly clarified.

```

Apenas dois atributos são opcionais: a URL da página de login e o título do relatório. Se a URL de login não for fornecida, o framework utiliza o spider do ZAP para tentar identificar as URLs da aplicação a partir da URL base. No entanto, nem sempre é possível encontrar a página de login automaticamente, por isso é recomendável fornecer essa URL sempre que possível. Se o spider falhar em encontrar a URL de login, um erro será lançado.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the contents of example.json here and remove the file from disk. Explain each of the fields. And provide an actual valid configuration... maybe for the Juice Shop application running on a container or VM.

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included a real-world example in the documentation, something like this.

    context: "context_bWAPP"
    url: "192.168.15.3/bWAPP/"
    url_login: "192.168.15.3/bWAPP/login"
    exclude_urls: []
    report_title: "Report_bWAPP"
    login: "admin"
    password: "admin"


## Web Server

O servidor web foi projetado para criar uma interface entre o framework e o usuário. Ele roda na porta `8000` e expõe o volume no localhost, permitindo que o usuário visualize os arquivos gerados.
Copy link
Member

@cunha cunha Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a local folder is shared between the host and containers, then there is no need for a server to host the files. (See comments in docker-compose.yml and copy.sh.)

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As requested, the server was removed from the final version of the framework.

}
```

O Pydantic fará o *parser* do arquivo e extrairá os atributos necessários para a criação do contexto. Um exemplo será disponibilizado na raiz do repositório, chamado `example.json`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, mover essa parte para o ponto acima onde descrevemos a classe do Pydantic. Acho que se mantivermos o example.json é melhor colocá-lo dentro do ./shared de uma vez.

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it in the root of the application because the example is only copied to the volume when the application starts. How would the user have access to it before that?

Para iniciar o framework, utilize o comando:

```bash
make run DIR="diretório dos arquivos"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quais arquivos?

I think we can make this process much simpler using the ./shared trick.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see https://github.com/cunha/easymapit for an example of a container that uses a shared directory to receive input and write output. It doesn't user Docker Compose, but the same mechanism applies. With plain Docker, the -v is the switch we use to configure the volume in the container.

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make it as user-friendly as possible. The user can have a folder anywhere in their workspace with the necessary files for running the tests. Once again, the documentation explains the process.

O `docker-compose` irá subir três containers: o ZAP, o framework e o servidor web. Os containers rodam em *background*. Para visualizar os logs, use os comandos:

```bash
docker logs -f authentication_zap_gt_crivo-zaproxy-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use plain docker logs instead of docker compose logs <service>?

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Zap container generates a lot of output on the screen, so I preferred to separate both containers to make debugging clearer.

@@ -0,0 +1,28 @@
# Use a imagem oficial do Python como base
Copy link
Member

@cunha cunha Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could try to drop this container entirely by using a shared volume.

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand the purpose.

@@ -0,0 +1,29 @@
annotated-types==0.7.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems to include everything from a pip freeze, but it would be better, for documentation reasons, to list only the dependencies. If you want to lock the version of all dependencies, then we could use a project management tool (I hear rye is the hot thing these days) to create a lock file for us.

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed libraries from the language and tried to keep the ones that are used through imports.

FIREFOX = os.getenv("FIREFOX")
ZAP_API_KEY = os.getenv("ZAP_API_KEY")
ZAP_PROXY_ADDRESS = os.getenv("ZAP_PROXY_ADDRESS")
ZAP_PROXY_PORT = 8080
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use getenv for the port number too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can provide a default with os.getenv("ZAP_PROXY_PORT", 8080)

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! The environment variable is treated as an integer, so I just performed a type conversion, something like:

ZAP_PROXY_PORT = int(os.getenv("ZAP_PROXY_PORT"))

for element in elements:
element_info = {}
try:
attributes_to_gather = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Esses atributos são do HTML, certo? A pessoa tem flexibilidade para definir estes atributos ou são fixos do padrão HTML?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the attributes can be generated by the developer, then should we have regexes to find the attributes too?

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of the framework uses only HTML tags.

depends_on:
- zaproxy
volumes:
- ./framework:/app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The framework directory is already copied inside the container at /app by the Dockerfile, is this doing something different or the same thing?

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to separate the setup of the two containers, the framework and ZAP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, can you please elaborate?

password, credential_password, 1
)
else:
if "%40" in text:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cryptic. Why do we only replace @ with %40 when login != password? Please write some documentation explaining what is being accomplished here and why it is needed.

Copy link
Contributor Author

@Sacramento-20 Sacramento-20 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explain what happens in the documentation: in some applications, the request body formats in a way that the @ becomes %40.

print("failed to log in !")
driver.refresh()
return
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid naked except clauses. It doesn't seem like the text above raises any exception, and the code seems also unclear how the function "returns" information to the caller (it either prints "failed to log in" or "login done"). How do we know what has happened outside the function?). Maybe rewrite the code so the intent is clearer or write some documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit seems unrelated to the issue here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function handles cases where the element cannot be found. If one of the elements corresponding to the form is incompatible, it will refresh the page to apply other elements during authentication.

Copy link
Member

@cunha cunha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comments inline on suggestions to improve. The most significant one is the handling of the volume for exchanging inputs and outputs with the container, which will significantly simplify usage.

@Sacramento-20 Sacramento-20 requested a review from cunha December 23, 2024 21:52
Copy link
Member

@cunha cunha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parei a revisão na metade. Todos os comentários foram fechados com ponteiros para commits muitas vezes não relacionados ao comentário que eu tinha deixado. Se for possível, passe nos comentários que eu tinha feito elaborando as respostas ou fazendo modificações para adequar o código.

depends_on:
- zaproxy
volumes:
- ./framework:/app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, can you please elaborate?

print("failed to log in !")
driver.refresh()
return
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit seems unrelated to the issue here

credential_login="{%username%}",
credential_password="{%password%}",
):
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For docscrings, we should follow https://peps.python.org/pep-0257 so autogenerated documentation matches existing formatting templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i forgot to use black in the code, now its done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants